feat: forward txs to sequencer #1208
Conversation
WalkthroughThis update introduces a new CLI flag for configuring a sequencer HTTP endpoint used in transaction gossiping. It extends the Ethereum backend to optionally forward transactions to a remote sequencer RPC service and conditionally disables local transaction pool persistence. Additional transaction gossip control flags and related configuration fields are added, along with version bumping. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant Node
participant EthAPIBackend
participant SequencerRPC
participant TxPool
User->>CLI: Start node with sequencer HTTP flag
CLI->>Node: Initialize with config
Node->>EthAPIBackend: Pass gossip flags and sequencer endpoint
User->>EthAPIBackend: SendTx(signedTx)
EthAPIBackend->>EthAPIBackend: Check tx type
alt BlobTxType
EthAPIBackend-->>User: Return error
else sequencerRPC configured
EthAPIBackend->>EthAPIBackend: AddLocal(tx)
alt AddLocal fails
EthAPIBackend-->>User: Return error
else
EthAPIBackend->>SequencerRPC: Forward tx via eth_sendRawTransaction
alt Forwarding fails and disableTxPool
EthAPIBackend-->>User: Return error
else Forwarding fails but disableTxPool false
EthAPIBackend-->>User: Return success (log warning)
else Forwarding succeeds
EthAPIBackend-->>User: Return success
end
end
else
EthAPIBackend->>TxPool: AddLocal(tx)
EthAPIBackend-->>User: Return result/error
end
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (4)
🧰 Additional context used🧠 Learnings (1)eth/api_backend.go (1)⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (5)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
eth/handler.go (1)
442-444: Fix potential nil pointer dereference in Stop method.The Stop method unconditionally calls
h.txsSub.Unsubscribe(), buth.txsSubwill be nil whendisableTxBroadcastis true since the subscription is not created in that case.Add a nil check to prevent panic:
func (h *handler) Stop() { - h.txsSub.Unsubscribe() // quits txBroadcastLoop + if h.txsSub != nil { + h.txsSub.Unsubscribe() // quits txBroadcastLoop + } h.minedBlockSub.Unsubscribe() // quits blockBroadcastLoop
🧹 Nitpick comments (1)
eth/backend.go (1)
311-319: Consider making the connection timeout configurable.The hardcoded 5-second timeout for the sequencer RPC connection may be insufficient for high-latency networks or during network congestion.
Consider adding a configuration option for the timeout:
- ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + timeout := 5 * time.Second + if config.TxGossipSequencerTimeout > 0 { + timeout = config.TxGossipSequencerTimeout + } + ctx, cancel := context.WithTimeout(context.Background(), timeout)Additionally, consider validating the HTTP endpoint format before attempting the connection to provide better error messages.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
cmd/geth/main.go(1 hunks)cmd/geth/usage.go(1 hunks)cmd/utils/flags.go(2 hunks)eth/api_backend.go(4 hunks)eth/backend.go(5 hunks)eth/ethconfig/config.go(1 hunks)eth/handler.go(4 hunks)eth/handler_eth.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: test
- GitHub Check: check
- GitHub Check: semgrep/ci
- GitHub Check: Analyze (go)
🔇 Additional comments (16)
eth/ethconfig/config.go (1)
234-236: LGTM! Clean configuration additions.The new configuration fields for transaction gossip control are well-named, properly typed, and appropriately placed in the Config struct.
eth/handler_eth.go (1)
59-61: LGTM! Correct implementation of transaction receiving control.The early return when
disableTxReceivingis true ensures that transaction receiving can be completely disabled, which aligns with the PR objective.cmd/geth/main.go (1)
179-181: LGTM! Proper CLI flag integration.The three transaction gossip control flags are correctly added to the node configuration flags, making them available as command-line options.
eth/handler.go (4)
97-98: LGTM! Clean configuration structure additions.The new fields for controlling transaction gossip behavior are properly added to the handler configuration.
138-139: LGTM! Proper field additions to handler struct.The transaction gossip control fields are correctly added to the handler struct.
159-160: LGTM! Correct field initialization.The configuration fields are properly initialized in the handler constructor.
425-430: LGTM! Proper conditional initialization of transaction broadcast.The transaction broadcast loop initialization is correctly guarded by the
disableTxBroadcastflag, preventing unnecessary resource allocation when broadcasting is disabled.eth/backend.go (4)
112-114: LGTM: Sequencer RPC service field addition.The new
seqRPCServicefield is properly typed and documented. The placement within the "Scroll additions" section is appropriate.
289-290: LGTM: Handler configuration for transaction gossip control.The new configuration flags for disabling transaction broadcast and receiving are properly integrated into the handler configuration.
300-300: LGTM: EthAPIBackend initialization with transaction gossip flag.The addition of
config.TxGossipReceivingDisabledparameter properly integrates the new transaction pool control functionality.
693-695: LGTM: Proper resource cleanup for sequencer RPC service.The cleanup logic correctly checks for nil and closes the RPC client to prevent resource leaks.
eth/api_backend.go (3)
28-28: LGTM: Required imports for sequencer functionality.The added imports for
hexutilandlogare necessary for transaction encoding and logging in the new sequencer forwarding logic.Also applies to: 39-39
49-49: LGTM: Transaction pool control field addition.The
disableTxPoolfield provides clear control over local transaction pool behavior.
295-298: LGTM: Clean encapsulation of local transaction pool logic.The
sendTxhelper method properly encapsulates the transaction pool addition logic with appropriate validation.cmd/utils/flags.go (2)
896-909: LGTM: Well-structured flag definitions.The three new transaction gossip flags are properly defined with clear names, appropriate types, and descriptive usage strings. The flag naming follows the established conventions in the codebase.
1807-1814: LGTM: Correct flag-to-config mapping with appropriate logging.The boolean flag assignments are implemented correctly, following the established pattern of other flags in this function. The informational logging provides useful visibility into the configuration state.
yiweichi
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
eth/api_backend.go (1)
267-297: Review error handling and transaction flow logic.The
SendTxmethod has the same concerning behavior patterns identified in previous reviews:
Blob transaction rejection timing: Blob transactions are rejected before attempting sequencer forwarding, which prevents forwarding even if the sequencer supports them.
Error suppression concern: When sequencer forwarding succeeds but local pool addition fails, the error is logged as a warning and
nilis returned, potentially misleading users.The suggested alternative flow from the previous review remains valid and should be implemented.
🧹 Nitpick comments (1)
eth/api_backend.go (1)
49-49: Consider consistent field alignment.The
disableTxPoolfield should be aligned with the other fields in the struct for better code consistency.type EthAPIBackend struct { extRPCEnabled bool allowUnprotectedTxs bool - disableTxPool bool + disableTxPool bool eth *Ethereum gpo *gasprice.Oracle }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
eth/api_backend.go(4 hunks)params/version.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- params/version.go
🧰 Additional context used
🧠 Learnings (1)
eth/api_backend.go (1)
Learnt from: Thegaram
PR: scroll-tech/go-ethereum#1204
File: miner/scroll_worker.go:836-845
Timestamp: 2025-06-11T15:54:05.820Z
Learning: When `processTxn` rejects a transaction because its calculated L1 data fee is ≥ `fees.MaxL1DataFee()`, this indicates a mis-configured system parameter, not a bad transaction, so the tx must remain in the TxPool rather than being purged.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (2)
eth/api_backend.go (2)
28-28: LGTM: Required imports added correctly.The
hexutilandlogimports are properly added to support the new transaction forwarding functionality.Also applies to: 39-39
299-302: LGTM: Helper method is well-implemented.The
sendTxhelper method properly encapsulates the local transaction pool logic and maintains the existing validation behavior.
There was a problem hiding this comment.
lgtm. There's a CI failure here: https://github.com/scroll-tech/go-ethereum/actions/runs/16101769286/job/45431817487.
This looks like a long-existing CI issue we used to have. I remember I discussed this with Hongfan, and we either ignore this CI failure or remove it. |
Can still run |
1. Purpose or design rationale of this PR
This PR add feature to enable RPC node forward transactions directly to sequencer.
txgossip.sequencerhttp: the sequencer's http rpc endpoint2. PR title
Your PR title must follow conventional commits (as we are doing squash merge for each PR), so it must start with one of the following types:
3. Deployment tag versioning
Has the version in
params/version.gobeen updated?4. Breaking change label
Does this PR have the
breaking-changelabel?Summary by CodeRabbit
New Features
Bug Fixes
Chores